Experimental: Selenium Grid scaler in K8s implementation preview #2400

VietND96 · 2024-09-19T13:01:06Z

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Introduction

Selenium Grid Scaler is a built-in scaler is maintained in upstream KEDA repository. The scaler implementation could be found here. The official docs of the scaler could be seen here.

Now, SeleniumHQ/docker-selenium involves as the maintainer for the scaler.

In order to deliver and get feedback continuously on any new bug fixes, improvement, or features for the Selenium Grid scaler. We select the latest stable version of KEDA core, patch the scaler implementation then build and deploy KEDA container images following our image tag convention.

The stable implementation will be merged to the upstream KEDA repository frequently and will be available in the next KEDA core release.

Read more: https://github.com/SeleniumHQ/docker-selenium/blob/trunk/.keda/README.md

Fixes #1925
Fixes #2160
Fixes #1904

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

PR Type

Enhancement, Documentation, Tests, Other

Description

Implemented Selenium Grid Scaler logic and integrated it with KEDA for scaling based on Selenium Grid metrics.
Added comprehensive test cases for Selenium Grid Scaler covering various scenarios.
Updated KEDA image configuration and Helm chart settings with Selenium-specific parameters.
Added documentation for Selenium Grid Scaler usage, configuration, and integration with KEDA.
Updated deployment workflow to include Selenium Grid scaler resources.
Introduced new environment variables and updated default values for KEDA configurations.

Changes walkthrough 📝

Relevant files

Configuration changes

5 files

chart_test.sh `Update KEDA Helm chart configuration and defaults.` tests/charts/make/chart_test.sh Added new environment variables `KEDA_BASED_NAME` and `KEDA_BASED_TAG`. Changed default value of `TEST_EXISTING_KEDA` to "false". Updated Helm commands to use new KEDA image settings.	+15/-3
CONFIGURATION.md `Update KEDA image configuration in Selenium Grid chart.` charts/selenium-grid/CONFIGURATION.md Updated KEDA image configuration with Selenium-specific settings.	+4/-1
values.yaml `Configure KEDA components and enable webhooks` charts/selenium-grid/values.yaml Added configuration for KEDA image specifications. Enabled KEDA admission webhooks component.	+17/-1
deploy.yml `Update deployment workflow to include grid scaler resources` .github/workflows/deploy.yml Added step to fetch Selenium Grid scaler resources in the deployment workflow.	+3/-1
base-auth-ingress-values.yaml `Enable KEDA webhooks in base auth ingress configuration` tests/charts/ci/base-auth-ingress-values.yaml Enabled KEDA webhooks in base authentication ingress values.	+4/-0

Enhancement

3 files

update_tag_in_docs_and_files.sh `Add KEDA tag version update logic.` update_tag_in_docs_and_files.sh Added logic to update KEDA tag versions in files.	+5/-0
selenium_grid_scaler.go `Implement Selenium Grid Scaler for KEDA integration.` .keda/scalers/selenium_grid_scaler.go Implemented Selenium Grid Scaler logic. Added functions for parsing metadata and counting sessions. Integrated with KEDA for scaling based on Selenium Grid metrics.	+379/-0
Makefile `Add Selenium Grid scaler integration and release processes` Makefile Added variables for KEDA versioning and naming. Introduced new targets for fetching and releasing Selenium Grid scaler resources and images. Integrated grid scaler release steps into existing release processes. Updated chart test configurations to include KEDA parameters.	+41/-10

Documentation

4 files

generate_chart_changelog.sh `Add experimental section for Selenium Grid Scaler.` generate_chart_changelog.sh Added a new "Experimental" section to the changelog for Selenium Grid Scaler.	+4/-0
README.md `Document Selenium Grid Scaler usage and configuration.` .keda/README.md Added documentation for Selenium Grid Scaler usage and configuration. Included instructions for using patched KEDA images.	+62/-0
selenium-grid-scaler.md `Document Selenium Grid Scaler configuration and usage` .keda/scalers/selenium-grid-scaler.md Added documentation for Selenium Grid Scaler. Included trigger specification and example configurations. Provided authentication parameters and examples.	+207/-0
README.md `Update README with Selenium Grid scaler preview information` charts/selenium-grid/README.md Added note about previewing new changes in Selenium Grid scaler implementation.	+2/-0

Tests

2 files

selenium_grid_scaler_test.go `Add test cases for Selenium Grid Scaler functionality.` .keda/scalers/selenium_grid_scaler_test.go Added comprehensive test cases for Selenium Grid Scaler. Tests cover various scenarios for session queue requests and node capabilities.	+1808/-0
DeploymentAutoscaling-values.yaml `Adjust max replica count in deployment autoscaling test` tests/charts/ci/DeploymentAutoscaling-values.yaml Increased max replica count for deployment autoscaling test.	+1/-1

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro · 2024-09-19T13:25:19Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review Test Coverage Extensive new test cases have been added for the Selenium Grid scaler. These tests cover various scenarios and edge cases. The reviewer should carefully examine these tests to ensure they adequately cover the scaler's functionality and potential failure modes. Documentation Update New documentation has been added explaining the Selenium Grid Scaler, its maintenance, and how to use the patched version. The reviewer should ensure this documentation is clear, accurate, and provides sufficient guidance for users. Configuration Changes The KEDA configuration in the Helm chart values has been updated to use Selenium-specific images. The reviewer should verify these changes are correct and consistent with the new scaler implementation.

codiumai-pr-agent-pro · 2024-09-19T13:26:06Z

PR Code Suggestions ✨

Category	Suggestion	Score
Performance	Use maps instead of slices for faster node lookup operations Consider using a more efficient data structure, such as a map, for `reservedNodes` and `newRequestNodes` to improve lookup performance when checking for existing nodes. .keda/scalers/selenium_grid_scaler.go [310-312] -var reservedNodes []ReservedNodes -var newRequestNodes []ReservedNodes +reservedNodes := make(map[string]ReservedNodes) +newRequestNodes := make(map[string]ReservedNodes) Apply this suggestion Suggestion importance[1-10]: 9 Why: Using maps instead of slices for node lookups is a significant performance improvement, especially when dealing with large datasets, making this a valuable suggestion.	9
Maintainability	Use a function to extract version numbers from the Makefile to reduce code duplication Consider using a function to extract version numbers from the Makefile to reduce code duplication and improve maintainability. update_tag_in_docs_and_files.sh [8-12] -FFMPEG_TAG_PREV_VERSION=$(grep FFMPEG_TAG_PREV_VERSION Makefile \| sed 's/.,$[^)]$)/\1/p' \| head -n 1) -FFMPEG_TAG_VERSION=$(grep FFMPEG_TAG_VERSION Makefile \| sed 's/.,$[^)]$)/\1/p' \| head -n 1) -RCLONE_TAG_VERSION=$(grep RCLONE_TAG_VERSION Makefile \| sed 's/.,$[^)]$)/\1/p' \| head -n 1) -KEDA_TAG_PREV_VERSION=$(grep KEDA_TAG_PREV_VERSION Makefile \| sed 's/.,$[^)]$)/\1/p' \| head -n 1) -KEDA_TAG_VERSION=$(grep KEDA_TAG_VERSION Makefile \| sed 's/.,$[^)]$)/\1/p' \| head -n 1) +get_version() { + grep "$1" Makefile \| sed 's/.,$[^)]$)/\1/p' \| head -n 1 +} +FFMPEG_TAG_PREV_VERSION=$(get_version FFMPEG_TAG_PREV_VERSION) +FFMPEG_TAG_VERSION=$(get_version FFMPEG_TAG_VERSION) +RCLONE_TAG_VERSION=$(get_version RCLONE_TAG_VERSION) +KEDA_TAG_PREV_VERSION=$(get_version KEDA_TAG_PREV_VERSION) +KEDA_TAG_VERSION=$(get_version KEDA_TAG_VERSION) + Apply this suggestion Suggestion importance[1-10]: 8 Why: Introducing a function to extract version numbers reduces code duplication and enhances maintainability. This change simplifies the code and makes it easier to manage, which is beneficial for long-term maintenance.	8
	Use a template function to reduce repetition in grid scaler release targets Consider using a loop or a function to reduce repetition in the 'release_grid_scaler', 'release_grid_scaler_latest', and 'release_grid_scaler_nightly' targets. Makefile [261-274] -release_grid_scaler: fetch_grid_scaler_images - docker buildx imagetools create -t $(NAME)/keda:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG) - docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG) - docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG) +define release_grid_scaler_template +release_grid_scaler$(1): fetch_grid_scaler_images + docker buildx imagetools create -t $(NAME)/keda$(2) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG) + docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver$(2) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG) + docker buildx imagetools create -t $(NAME)/keda-admission-webhooks$(2) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG) +endef -release_grid_scaler_latest: fetch_grid_scaler_images - docker buildx imagetools create -t $(NAME)/keda:latest $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG) - docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:latest $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG) - docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:latest $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG) +$(eval $(call release_grid_scaler_template,,:$(KEDA_TAG_VERSION)-$(BUILD_DATE))) +$(eval $(call release_grid_scaler_template,_latest,:latest)) +$(eval $(call release_grid_scaler_template,_nightly,:nightly)) -release_grid_scaler_nightly: fetch_grid_scaler_images - docker buildx imagetools create -t $(NAME)/keda:nightly $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG) - docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:nightly $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG) - docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:nightly $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG) - Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion effectively reduces code repetition and enhances maintainability by using a template function, which is a significant improvement for code clarity and future updates.	8
	Use an array to store KEDA image settings for improved maintainability and reduced repetition Consider using an array to store the KEDA image settings and then join them in the Helm command. This approach would make the code more maintainable and reduce repetition. tests/charts/make/chart_test.sh [340-344] -HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} \ ---set keda.image.keda.registry=${KEDA_BASED_NAME} --set keda.image.keda.repository=keda --set keda.image.keda.tag=${KEDA_BASED_TAG} \ ---set keda.image.metricsApiServer.registry=${KEDA_BASED_NAME} --set keda.image.metricsApiServer.repository=keda-metrics-apiserver --set keda.image.metricsApiServer.tag=${KEDA_BASED_TAG} \ ---set keda.image.webhooks.registry=${KEDA_BASED_NAME} --set keda.image.webhooks.repository=keda-admission-webhooks --set keda.image.webhooks.tag=${KEDA_BASED_TAG} \ -" +keda_images=( + "keda.image.keda.registry=${KEDA_BASED_NAME}" + "keda.image.keda.repository=keda" + "keda.image.keda.tag=${KEDA_BASED_TAG}" + "keda.image.metricsApiServer.registry=${KEDA_BASED_NAME}" + "keda.image.metricsApiServer.repository=keda-metrics-apiserver" + "keda.image.metricsApiServer.tag=${KEDA_BASED_TAG}" + "keda.image.webhooks.registry=${KEDA_BASED_NAME}" + "keda.image.webhooks.repository=keda-admission-webhooks" + "keda.image.webhooks.tag=${KEDA_BASED_TAG}" +) +HELM_COMMAND_SET_IMAGES="${HELM_COMMAND_SET_IMAGES} $(printf -- '--set %s ' "${keda_images[@]}")" Apply this suggestion Suggestion importance[1-10]: 7 Why: Using an array to store the KEDA image settings improves maintainability by reducing repetition and making the code easier to update in the future. This is a good practice for cleaner code, though it doesn't address a critical issue.	7
	Encapsulate parameters in a struct to improve function signature readability Consider using a struct to encapsulate the parameters passed to the `checkCapabilitiesMatch` function to improve readability and maintainability. .keda/scalers/selenium_grid_scaler.go [250] -func checkCapabilitiesMatch(capability Capability, requestCapability Capability, browserName string, browserVersion string, sessionBrowserName string, platformName string) bool { +type CapabilityMatchParams struct { + capability Capability + requestCapability Capability + browserName string + browserVersion string + sessionBrowserName string + platformName string +} +func checkCapabilitiesMatch(params CapabilityMatchParams) bool { + Apply this suggestion Suggestion importance[1-10]: 7 Why: Encapsulating parameters in a struct improves readability and maintainability of the function signature, but it does not address any critical issues.	7
	Use a heredoc for the multi-line Helm command to improve readability Consider using a heredoc for the multi-line Helm command to improve readability and maintainability. tests/charts/make/chart_test.sh [334-338] -helm upgrade -i ${KEDA_NAMESPACE} -n ${KEDA_NAMESPACE} --create-namespace --set webhooks.enabled=true \ ---set image.keda.registry=${KEDA_BASED_NAME} --set image.keda.repository=keda --set image.keda.tag=${KEDA_BASED_TAG} \ ---set image.metricsApiServer.registry=${KEDA_BASED_NAME} --set image.metricsApiServer.repository=keda-metrics-apiserver --set image.metricsApiServer.tag=${KEDA_BASED_TAG} \ ---set image.webhooks.registry=${KEDA_BASED_NAME} --set image.webhooks.repository=keda-admission-webhooks --set image.webhooks.tag=${KEDA_BASED_TAG} \ -kedacore/keda +helm upgrade -i ${KEDA_NAMESPACE} -n ${KEDA_NAMESPACE} --create-namespace kedacore/keda <<EOF + --set webhooks.enabled=true + --set image.keda.registry=${KEDA_BASED_NAME} + --set image.keda.repository=keda + --set image.keda.tag=${KEDA_BASED_TAG} + --set image.metricsApiServer.registry=${KEDA_BASED_NAME} + --set image.metricsApiServer.repository=keda-metrics-apiserver + --set image.metricsApiServer.tag=${KEDA_BASED_TAG} + --set image.webhooks.registry=${KEDA_BASED_NAME} + --set image.webhooks.repository=keda-admission-webhooks + --set image.webhooks.tag=${KEDA_BASED_TAG} +EOF Apply this suggestion Suggestion importance[1-10]: 6 Why: Using a heredoc for multi-line commands can enhance readability and maintainability, making the code easier to understand and modify. However, this suggestion is more about style and readability than addressing a functional issue.	6
Enhancement	Add more comprehensive test cases to cover edge cases and diverse scenarios Consider adding more specific test cases that cover edge cases and boundary conditions, such as testing with maximum allowed sessions, or with a mix of different browser versions and platform names. .keda/scalers/selenium_grid_scaler_test.go [21-42] tests := []struct { name string args args want int64 wantErr bool }{ // ... existing test cases ... + { + name: "max allowed sessions reached", + args: args{ + b: []byte(`{"data":{"grid":{"maxSession":10},"sessionsInfo":{"sessionQueueRequests":["","","","","","","","","","",""]}}}`), + browserName: "chrome", + sessionBrowserName: "chrome", + browserVersion: "latest", + platformName: "linux", + nodeMaxSessions: 10, + }, + want: 1, + wantErr: false, + }, + { + name: "mix of different browser versions and platforms", + args: args{ + b: []byte(`{"data":{"grid":{},"sessionsInfo":{"sessionQueueRequests":["{\"browserName\":\"chrome\",\"browserVersion\":\"91.0\",\"platformName\":\"windows\"}","{\"browserName\":\"chrome\",\"browserVersion\":\"92.0\",\"platformName\":\"linux\"}"]}}}`), + browserName: "chrome", + sessionBrowserName: "chrome", + browserVersion: "latest", + platformName: "any", + nodeMaxSessions: 5, + }, + want: 2, + wantErr: false, + }, } Suggestion importance[1-10]: 8 Why: Adding more specific test cases for edge cases and boundary conditions is important for ensuring robustness and reliability of the code, making this a valuable enhancement.	8
	Refactor test cases to use a more concise and maintainable table-driven test structure Consider using table-driven tests to reduce code duplication and improve test maintainability. This approach allows for easier addition of new test cases and better organization of test data. .keda/scalers/selenium_grid_scaler_test.go [21-42] tests := []struct { - name string - args args - want int64 - wantErr bool + name string + responseBody []byte + browserName string + expectedCount int64 + expectedError bool }{ { - name: "nil response body should throw error", - args: args{ - b: []byte(nil), - browserName: "", - }, - wantErr: true, + name: "nil response body should throw error", + responseBody: nil, + browserName: "", + expectedCount: 0, + expectedError: true, }, { - name: "empty response body should throw error", - args: args{ - b: []byte(""), - browserName: "", - }, - wantErr: true, + name: "empty response body should throw error", + responseBody: []byte(""), + browserName: "", + expectedCount: 0, + expectedError: true, }, // ... more test cases ... } Suggestion importance[1-10]: 7 Why: The suggestion to refactor the test cases into a more concise table-driven format improves readability and maintainability, making it easier to add new test cases. However, it is not crucial for functionality.	7
	Introduce a variable for the common Docker image name prefix to improve maintainability and reduce repetition Consider using a variable for the common Docker image name prefix 'selenium/' to improve maintainability and reduce repetition. Makefile [262-264] -docker buildx imagetools create -t $(NAME)/keda:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG) -docker buildx imagetools create -t $(NAME)/keda-metrics-apiserver:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG) -docker buildx imagetools create -t $(NAME)/keda-admission-webhooks:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG) +DOCKER_IMAGE_PREFIX := $(NAME)/ +docker buildx imagetools create -t $(DOCKER_IMAGE_PREFIX)keda:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda:$(KEDA_BASED_TAG) +docker buildx imagetools create -t $(DOCKER_IMAGE_PREFIX)keda-metrics-apiserver:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-metrics-apiserver:$(KEDA_BASED_TAG) +docker buildx imagetools create -t $(DOCKER_IMAGE_PREFIX)keda-admission-webhooks:$(KEDA_TAG_VERSION)-$(BUILD_DATE) $(KEDA_BASED_NAME)/keda-admission-webhooks:$(KEDA_BASED_TAG) Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to use a variable for the Docker image name prefix is valid as it reduces repetition and improves maintainability, but it is not a critical change.	7
Best practice	Introduce a constant for the default node max sessions value to improve flexibility and readability Consider using a constant or a variable for the `nodeMaxSessions` value in the `args` struct, as it's currently hardcoded and might need to be adjusted for different test cases. .keda/scalers/selenium_grid_scaler_test.go [13-20] +const defaultNodeMaxSessions = 5 + type args struct { b []byte browserName string sessionBrowserName string browserVersion string platformName string nodeMaxSessions int } Apply this suggestion Suggestion importance[1-10]: 6 Why: Using a constant for `nodeMaxSessions` enhances code readability and flexibility, allowing for easier adjustments in the future. It is a good practice but not critical for the current implementation.	6

💡 Need additional feedback ? start a PR chat

Signed-off-by: Viet Nguyen Duc <[email protected]>

VietND96

Merged

VietND96 force-pushed the trunk branch from 678c139 to a0476ae Compare September 19, 2024 13:22

VietND96 marked this pull request as ready for review September 19, 2024 13:24

codiumai-pr-agent-pro bot added enhancement tests documentation Other Review effort [1-5]: 3 labels Sep 19, 2024

VietND96 force-pushed the trunk branch 2 times, most recently from fc9f8ff to 0ced319 Compare September 19, 2024 14:12

Experimental: Selenium Grid scaler in K8s implementation preview

a8c2fe1

Signed-off-by: Viet Nguyen Duc <[email protected]>

VietND96 force-pushed the trunk branch from 0ced319 to a8c2fe1 Compare September 19, 2024 14:14

VietND96 commented Sep 19, 2024

View reviewed changes

VietND96 merged commit 2af6166 into SeleniumHQ:trunk Sep 19, 2024
29 of 33 checks passed

VietND96 added this to the 4.25.0 milestone Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental: Selenium Grid scaler in K8s implementation preview #2400

Experimental: Selenium Grid scaler in K8s implementation preview #2400

VietND96 commented Sep 19, 2024 •

edited

Loading

codiumai-pr-agent-pro bot commented Sep 19, 2024

codiumai-pr-agent-pro bot commented Sep 19, 2024

VietND96 left a comment

Experimental: Selenium Grid scaler in K8s implementation preview #2400

Experimental: Selenium Grid scaler in K8s implementation preview #2400

Conversation

VietND96 commented Sep 19, 2024 • edited Loading

User description

Description

Motivation and Context

Introduction

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

codiumai-pr-agent-pro bot commented Sep 19, 2024

PR Reviewer Guide 🔍

codiumai-pr-agent-pro bot commented Sep 19, 2024

PR Code Suggestions ✨

VietND96 left a comment

Choose a reason for hiding this comment

VietND96 commented Sep 19, 2024 •

edited

Loading